Import suggestion for missing newtype constructor, all types constructor and indirect overloadedrecorddot fields #4516
Conversation
guibou
force-pushed
the
import_suggestion_for_coerce
branch
from
bddf4b5 to
f1eb36a
Compare
|
Rebased and fixed test for GHC 9.4. |
guibou
force-pushed
the
import_suggestion_for_coerce
branch
2 times, most recently
from
01ce436 to
2b27964
Compare
|
I've added a new commit which fixs (and add tests) for not in scope record field. This was working fine with ghc <9.10 when the message was see:
The regex associated with |
guibou
force-pushed
the
import_suggestion_for_coerce
branch
from
ee52849 to
282e820
Compare
|
I'm not happy. Looks like I hit the difference in GHC 9.10 (and 9.8) about in which context fields name are found. Even if the I'll investigate a bit more. |
guibou
force-pushed
the
import_suggestion_for_coerce
branch
from
cb9d4e3 to
dcb6d3a
Compare
|
I finally found the solution (I hope), which is to use I've done this refactor, added a bit more tests, tell me what you think. Note that, maybe in a future refactor, we could try to be a bit more selective in the way we are looking for import. The Note also that some of the tests in this MR, related to field selector, are just testing that we can suggest to import the Type name (e.g. |
|
With ghc 9.10 ;) |
Maybe we can open issue to track the things we would want to do for imports. And hopefully tackle them one by one. |
In a context where we want to `coerce` to a type for which constructor
is not in scope, GHC fails with (e.g., for `Sum Int`):
```
• Couldn't match representation of type ‘Int’ with that of ‘Sum Int’
arising from a use of ‘coerce’
The data constructor ‘base-4.18.2.1:Data.Semigroup.Internal.Sum’
of newtype ‘Sum’ is not in scope
```
This code action detects the missing `newtype` and suggests to add the
required import.
This is convenient because otherwise the user need to interpret the
error message and most of the time manually find which module and type to import.
Note that a better implementation could try to decet that the type is
already imported (if that's the case) and just suggest to add the
constructor (e.g. `(..)`) in the import list, but this is too much
complexity to implement. It could lead to duplicated import lines which
will be "cleaned" by formatter or other extensions.
For example, `deriving instance Generic (Sum Int)`, but it should work
for other deriving which indirectly requires a complete access to the
type constructor.
```
Can't make a derived instance of ‘Generic (Sum Int)’:
The data constructors of ‘Sum’ are not all in scope
so you cannot derive an instance for it
```
For example, the following code `foo.titi` when the type of `foo` (e.g.
`Bar` here is not in scope and not from an already imported module (e.g.
the type exists indirectly because here `foo :: Bar` comes from another module).
If the module which contains `Bar` is already imported, GHC already
gives an hint to add `titi` to the `import Bar` line and this is already
correctly handled by HLS.
```
No instance for ‘HasField "titi" Bar.Bar String’
arising from selecting the field ‘titi’
```
This correct previous commit by handling ghc 9.4 parethensis instead of "tick".
In ghc 9.6 we had: ``` Not in scope ‘modelStatusTag’ ``` In 9.10 we have: ``` Not in scope: record field ‘modelStatusTag’ ``` Introducing the new regex in order to match it AND a test to ensure no future regression. The regression is due to the fact that there is a matcher which catch `Nat in scope: .*`, hence it was matching "record" instead of "modelStatusTag".
In GHC >= 9.8, the namespace for record selector changed and is now part of a new namespace. This allows for duplicated record field names in the same module. This hence generated a few issues in HLS when looking for a symbol using `lookupOccEnv`: the current implementation was only doing lookup in type, data and var namespaces. This commit uses `lookupOccEnv_AllNameSpaces`, so it may be more efficient (one lookup instead of two), but it also incluse the symbols from record field selectors and this will actually fix most import suggestion logic when a record field selector is not found. Note that the function is not available in `ghc` <= 9.6, hence the `CPP` and fallsback implementation, which uses the previous implementation. See https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.8#new-namespace-for-record-fields
guibou
force-pushed
the
import_suggestion_for_coerce
branch
from
dcb6d3a to
dc6a9e3
Compare
3 participants
This MR contains 3 commits which improves the import suggestion code actions:
newtypeconstructor is not in scope. This usually happen whencoerceingnewtype.deriving instance Generic BarwhenBar(the type) may be in scope, but not the construtors (e.g. requiringBar(..)).OverloadedRecordDotcontext, such asfoo.barwherefoo :: FooandFoo(bar)is not in scope. Note that this is already partially handled by HLS in the case where the module containingFoois already imported, but not the fields. In this context, GHC is kind enough to give us an hint (something like "maybe add fieldbarto import line...") and this is already handled by HLS. However, if the module containingFoois not already imported, it does not work.All of these case usually happen when types are "indirectly" in scope. For example, consider the followings:
In the module
Usage, the typeFoois not explicitly imported, it exists just indirectly becausefoois imported fromValue.Discussion
I tried to take care of multiples patterns in the regex used to match the error messages, such as qualified or not qualified types or class, as well as polymorphic or not. If you see a case not taken into account, please tell me.
Tests
I've added simple tests for the
coerceandinstance Generic"cases" and a more involved test for the overloaded record dot cases.I've developed this using GHC 9.10. I'll test on other version of GHC asap, perhaps some adaptations of the regex need to be introduced.